Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix[venom]: fix _stack_reorder() routine #4220

Merged
merged 13 commits into from
Oct 4, 2024

Conversation

HodanPlodky
Copy link
Collaborator

@HodanPlodky HodanPlodky commented Sep 2, 2024

What I did

Fixed the issue that was manifested in the snekmate test. This bug is reported in #4215

How I did it

The bug was caused by error in stack_reorder function. The function depends on the order of each operand but since the order changes there is the possibility to move same stack operand twice. To fix this issue I kept the information where necessary operands on stack.

How to verify it

The example from the issue does compile now.

Commit message

fix an issue where `stack_reorder()` reorders operands incorrectly,
resulting in the result stack not matching the target stack. this bug
can manifest when there are multiple copies of an operand on the stack.
in the `stack_reorder()` loop, an operand gets moved past one of its
copies which has not been moved yet, resulting in the operand getting
moved twice, instead of each copy of the operand getting moved once,
since `get_depth()` returns the wrong copy of the operand after the
first move.

this commit fixes the issue by keeping track of the positions of each
copy of each stack item, and ensuring that each copy only gets moved
once.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (e21f3e8) to head (76441a8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4220      +/-   ##
==========================================
- Coverage   91.36%   89.02%   -2.35%     
==========================================
  Files         108      108              
  Lines       15637    15655      +18     
  Branches     3440     3445       +5     
==========================================
- Hits        14287    13937     -350     
- Misses        920     1213     +293     
- Partials      430      505      +75     
Flag Coverage Δ
89.00% <100.00%> (-2.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HodanPlodky HodanPlodky marked this pull request as ready for review September 3, 2024 13:20
cost += self.swap(assembly, stack, depth)
if final_stack_depth in positions[stack.peek(final_stack_depth)]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the positions[stack.peek(final_stack_depth)] can be factored out into a variable

@charles-cooper charles-cooper changed the title fix[venom]: stack reorder rework fix[venom]: fix _stack_reorder() routine Sep 30, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) October 4, 2024 19:39
@charles-cooper charles-cooper merged commit 5d8280f into vyperlang:master Oct 4, 2024
156 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants